Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: blinking placeholder avatar on profile screen [WPB-3917] #2184

Merged
merged 6 commits into from
Sep 8, 2023
Merged

fix: blinking placeholder avatar on profile screen [WPB-3917] #2184

merged 6 commits into from
Sep 8, 2023

Conversation

saleniuk
Copy link
Contributor

@saleniuk saleniuk commented Sep 6, 2023

PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³

  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

When opening any profile screen, a placeholder image appears for a short amount of time before the proper avatar is loaded, even if the image is already cached.

Solutions

Provide a small delay of 200ms before showing the placeholder when loading proper avatar which is enough to fetch data from database and get the cached image, so it doesn't need to show the placeholder, only when the image is not already cached and needs to be downloaded, but even then it looks better together with the crossfade animation and new transition animations between screens.

Testing

How to Test

Open any profile screen.

Attachments (Optional)

These before/after videos are recorded with disabled transition animations to better see how exactly it looks.

Before After
avatars_before.mp4
profile_avatars_after.mp4
After with transitions
profile_avatars_after_with_transitions.mp4

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.
References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023 •
edited
Loading

Test Results

587 tests   586 ✔️  6m 48s ⏱️
  84 suites      1 💤
  84 files        0 ❌

Results for commit d387d98.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 602 succeeded.

The build produced the following APK's:

@saleniuk saleniuk requested review from a team, typfel, borichellow, MohamadJaara, vitorhugods, trOnk12 and Garzas and removed request for a team and trOnk12 September 6, 2023 16:09
vitorhugods
vitorhugods approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@vitorhugods vitorhugods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👨🏼‍🍳🤌🏻

@ohassine
Copy link
Member

ohassine commented Sep 7, 2023 •
edited
Loading

Should we merge it into RC ?

@saleniuk
Copy link
Contributor Author

saleniuk commented Sep 7, 2023

Should we merge it into RC ?

@ohassine do we currently have any restrictions on what fixes should be added to RC? 🤔
It's not a new feature and not any breaking change so I don't see any reason why not 😄

@ohassine
Copy link
Member

ohassine commented Sep 7, 2023

Should we merge it into RC ?

@ohassine do we currently have any restrictions on what fixes should be added to RC? 🤔
It's not a new feature and not any breaking change so I don't see any reason why not 😄

AFAIK, only flagged issues should go into RC

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

APKs built during tests are available here. Scroll down to Artifacts!

@saleniuk
Copy link
Contributor Author

saleniuk commented Sep 7, 2023

Should we merge it into RC ?

@ohassine do we currently have any restrictions on what fixes should be added to RC? 🤔
It's not a new feature and not any breaking change so I don't see any reason why not 😄

AFAIK, only flagged issues should go into RC

Currently we have only one flagged bug in our whole backlog so I don't think this rule applies. We were not supposed to merge any new features. 🤔

@AndroidBob
Copy link
Collaborator

Build 629 succeeded.

The build produced the following APK's:

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 645 succeeded.

The build produced the following APK's:

@saleniuk saleniuk merged commit 7a472ec into release/candidate Sep 8, 2023
10 checks passed
@saleniuk saleniuk deleted the fix/flickering_avatar_on_profile_screen branch September 8, 2023 09:01
github-actions bot pushed a commit that referenced this pull request Sep 8, 2023
@saleniuk @github-actions
fix: blinking placeholder avatar on profile screen [WPB-3917] (#2184)
8e70e5c
@github-actions github-actions bot mentioned this pull request Sep 8, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitorhugods vitorhugods vitorhugods approved these changes

@alexandreferris alexandreferris alexandreferris approved these changes

@typfel typfel Awaiting requested review from typfel typfel was automatically assigned from wireapp/android

@borichellow borichellow Awaiting requested review from borichellow borichellow was automatically assigned from wireapp/android

@MohamadJaara MohamadJaara Awaiting requested review from MohamadJaara MohamadJaara was automatically assigned from wireapp/android

@Garzas Garzas Awaiting requested review from Garzas

Assignees

@saleniuk saleniuk

Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@saleniuk @AndroidBob @ohassine @alexandreferris @vitorhugods